-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Resolve indices using original index pattern and retry the entire resolution #136009
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resolve indices using original index pattern and retry the entire resolution #136009
Conversation
# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java
# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java
# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/PreAnalyzer.java
| syncEsqlQueryRequest().query("FROM data,*:data | LOOKUP JOIN lookup ON key | WHERE f1 == 1") | ||
| .filter(new TermQueryBuilder("f2", 2)) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test would fail if we only retry previously resolved subset of clusters opposed to ones resolved to all clusters without the filter.
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
luigidellaquila
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as #134218, IMHO this is good
Thanks!
| } | ||
| })); | ||
| // retrying the index resolution without index filtering. | ||
| executionInfo.clusterInfo.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
smalyshev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine. I am not 100% happy with the fact that we do other resolutions twice in the case of filters, but I hope it'll go away when we fix field caps, so for now we can live with it I think.
| VerificationException.class, | ||
| containsString("lookup index [lookup] is not available in remote cluster [remote-b]"), | ||
| () -> runQuery( | ||
| syncEsqlQueryRequest().query("FROM data,*:data | LOOKUP JOIN lookup ON key | WHERE f1 == 1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder: please add the other use case without join we've discussed.
Today we parse original index pattern into EsqlExecutionInfo#clusterInfo and then assemble it back createIndexExpressionFromAvailableClusters. In CPS with flat expressions resolution logic is more complex (for example the list of remotes is no longer known beforehand). It should be delegated to field caps call using original expression. This also changes how resolution is retried. It is no longer trivial to exclude clusters with failures. Additionally
flatexpression could resolve to different set of remotes without filters. As a result we need to retry resolving dependencies such as enrich and lookup joins as well as we need to ensure they are present on a wider set of remotes.Resolves: ES-12487, ES-12978